Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

HDDS-9377. Implement some metrics with RW-lock instead of synchronization #7708

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

HDDS-9377. Implement some metrics with RW-lock instead of synchronization #7708

wants to merge 3 commits into from

Conversation

juncevich
Copy link
Contributor

What changes were proposed in this pull request?

Change synchronized metrics collect methods to read/write lock implementation.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-9377

How was this patch tested?

I wrote the JUnit test (I'm not sure I should add this test to this PR) to compare performance metric realization.

  @RepeatedTest(100)
  void testMyMetrics() throws InterruptedException {
    OzoneMutableRate rate = new OzoneMutableRate("Test name", "Test description", true);
    final MetricsRecordBuilder builder = getMetricsRecordBuilder();
    ExecutorService executor = Executors.newFixedThreadPool(10);
    for (int i = 0; i < 100; i++) {
      int finalI = i;

      executor.submit(() -> {
        rate.add(finalI);
        if (finalI % 100 == 0) {
          rate.snapshot(getMetricsRecordBuilder());
        }
      });
    }
    executor.awaitTermination(1, TimeUnit.SECONDS);
    rate.snapshot(builder);
    assertEquals(100, builder.metrics().get(0).value().intValue());
  }

@juncevich juncevich force-pushed the HDDS-9377 branch 2 times, most recently from 2f66e63 to 907cb96 Compare January 16, 2025 12:28
@juncevich juncevich marked this pull request as ready for review January 28, 2025 07:38
@adoroszlai adoroszlai changed the title HDDS-9377. Call MutableStat.add(long x) method as a runnable task in a separate service executor HDDS-9377. Implement metrics system with RW-lock instead of synchronization Jan 29, 2025
@adoroszlai adoroszlai requested a review from smengcl January 30, 2025 10:41
@adoroszlai
Copy link
Contributor

Thanks @juncevich for the patch. It seems this removes synchronization in some individual metrics, but not in MetricsSytemImpl itself. (see also HDDS-12193)

My 2 cents: I don't think we should reimplement Hadoop metrics system in Ozone with tweaks. Either improve it in Hadoop to benefit the ecosystem, or use a better existing metrics library in Ozone.

@adoroszlai adoroszlai changed the title HDDS-9377. Implement metrics system with RW-lock instead of synchronization HDDS-9377. Implement some metrics with RW-lock instead of synchronization Feb 4, 2025
@juncevich
Copy link
Contributor Author

Thanks @juncevich for the patch. It seems this removes synchronization in some individual metrics, but not in MetricsSytemImpl itself. (see also HDDS-12193)

My 2 cents: I don't think we should reimplement Hadoop metrics system in Ozone with tweaks. Either improve it in Hadoop to benefit the ecosystem, or use a better existing metrics library in Ozone.

Thanks for the notices. Could you tell me more about the better existing metrics library in Ozone? Is it an existing library in Ozone, or have you offered to change the metrics library from Hadoop to another metric system?

@adoroszlai
Copy link
Contributor

Could you tell me more about the better existing metrics library in Ozone? Is it an existing library in Ozone, or have you offered to change the metrics library from Hadoop to another metric system?

Not "existing in Ozone", I meant "existing somewhere else, which we could start using in Ozone".

  1. copy Hadoop's metrics system and improve
  2. improve in Hadoop
  3. replace

I consider (1) to be the worst option, both for Ozone and globally.

@juncevich
Copy link
Contributor Author

Could you tell me more about the better existing metrics library in Ozone? Is it an existing library in Ozone, or have you offered to change the metrics library from Hadoop to another metric system?

Not "existing in Ozone", I meant "existing somewhere else, which we could start using in Ozone".

  1. copy Hadoop's metrics system and improve
  2. improve in Hadoop
  3. replace

I consider (1) to be the worst option, both for Ozone and globally.

I think (2) is the best option, but it takes the longest. I will ask about not synchronizing metrics in the Hadoop project. And will return with the answer.

@juncevich
Copy link
Contributor Author

I looked at communication in Hadoop(mailing lists) and discussion and realization can take a long time )
I think the best way is not to depend on Hadoop libraries and change the metric library to another instead of improving the Hadoop metric library.
I offer to close this PR and create one task to change the metric library and (I'm not sure) another to create an Ozone metric API which can be implemented by any metric library.
What's your opinion, @adoroszlai ?

@kerneltime
Copy link
Contributor

@juncevich I have a few questions, what is the desired impact of this code change? Do you see a performance difference? How much is it? Answers to those questions will help us decide if it is ok to duplicate code but fix it to gain benefits. In the linked jira you do describe the problem but do not mention how much this PR will help solve the issue. Also, internal improvements sometimes do not lead to externally visible improvements, any details on how it helped speed up the test you'll deviced?

@juncevich
Copy link
Contributor Author

juncevich commented Feb 5, 2025 •
edited
Loading

@juncevich I have a few questions, what is the desired impact of this code change? Do you see a performance difference? How much is it? Answers to those questions will help us decide if it is ok to duplicate code but fix it to gain benefits. In the linked jira you do describe the problem but do not mention how much this PR will help solve the issue. Also, internal improvements sometimes do not lead to externally visible improvements, any details on how it helped speed up the test you'll deviced?

@kerneltime The desired impact is to get rid of the locks in metrics collecting and improve writing speed. Without a fix, it was a 3000-3200 writing operation (CreateFile or CommitKey). After fixing it was 3600-3700. Improvement of more than 12%.

@kerneltime
Copy link
Contributor

I looked at communication in Hadoop(mailing lists) and discussion and realization can take a long time ) I think the best way is not to depend on Hadoop libraries and change the metric library to another instead of improving the Hadoop metric library. I offer to close this PR and create one task to change the metric library and (I'm not sure) another to create an Ozone metric API which can be implemented by any metric library. What's your opinion, @adoroszlai ?

If we are to replace the metric implementation, I would go with something like dropwizard or another existing package to reuse after comparing performance.

@kerneltime The desired impact is to get rid of the locks in metrics collecting and improve writing speed. Without a fix, it was a 3000-3200 writing operation (CreateFile or CommitKey). After fixing it was 3600-3700. Improvement of more than 12%.

This is a decent improvement, @adoroszlai should we initiate an upstream discussion with Hadoop while we see if we can merge this change in. I think we need a dual strategy

  1. Evaluate a replacement for Hadoop metrics
  2. Start a PR upstream with Hadoop while we move Ozone to use the more performant solution.

@adoroszlai
Copy link
Contributor

I think we need a dual strategy

I agree.

Please also take a look at Ratis' metrics API, for which Ozone already uses the Dropwizard implementation.

@juncevich juncevich force-pushed the HDDS-9377 branch 2 times, most recently from 69a55f3 to 997884c Compare February 10, 2025 12:06
@juncevich juncevich force-pushed the HDDS-9377 branch 2 times, most recently from df94f80 to 0ac69a3 Compare February 10, 2025 19:32
adoroszlai
adoroszlai reviewed Feb 10, 2025
View reviewed changes
hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/ContainerClientMetrics.java Outdated
private MutableRate hsyncSynchronizedWorkNs;
private OzoneMutableRate hsyncSynchronizedWorkNs;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If possible, we should keep the original class names. Package name should be enough to distinguish between Hadoop and Ozone class. This would help reduce code change mostly to imports.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea! Will fix.

hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/metrics/OzoneMutableStat.java Outdated
* A convenient mutable metric for throughput measurement.
* (non-synchronized version of org.apache.hadoop.metrics2.lib.MutableStat)
*/
public class OzoneMutableStat extends MutableStat {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it supposed to extend MutableStat? This duplicates all member variables. Can we copy all necessary classes without subclassing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All metrics should extend MutableMetric. I will change extending MutableStat to MutableMetric.

hadoop-hdds/common/src/test/java/org/apache/hadoop/ozone/metric/SampleQuantilesTest.java Outdated
/**
* Tests for SampleQuantiles.
*/
class SampleQuantilesTest {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests named *Test are not executed:

ozone/pom.xml

Line 1844 in 6f82e7b

<include>**/Test*.java</include>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Fixed.

@juncevich juncevich force-pushed the HDDS-9377 branch 2 times, most recently from 35f0684 to 63b0cd3 Compare February 11, 2025 06:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adoroszlai adoroszlai adoroszlai left review comments

@kerneltime kerneltime Awaiting requested review from kerneltime

@jojochuang jojochuang Awaiting requested review from jojochuang

@smengcl smengcl Awaiting requested review from smengcl

Assignees
No one assigned
Labels
metrics performance
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@juncevich @adoroszlai @kerneltime